adds scoped values credential to support multiple endpoints. by ghyatzo · Pull Request #58 · JuliaClimate/CDSAPI.jl

ghyatzo · 2025-01-15T10:24:24Z

Hello,

I had wrote my script for downloading CDS data, before checking if it existed already.
Funnily enough our implementations are eerily similar.
There are some slight differences that I would like to propose as improvements/additions to this package so we can have a single point.

This is the first of 3 pull requests, one for each proposal so they can be addressed independently.

Ok with that out of the way:
I would like to propose the use of scoped values for the API credentials. As well as a bit better credential retrieval mechanism that also checks for environmental variables.

This allows to use different credentials for each request, for example:

cred1 = CDScredentials("path/to/first/set/of/credentials")
cred2 = CDScredentials("path/to/second/set/of/credentials")

with( auth => cred1) do
    CDSAPI.retrieve(name, request, target)
end

with( auth => cred2) do
    # makes the requests with a different token
    CDSAPI.retrieve(name, request, target)
end

# If none are provided, we look for credentials in the environmental variables
# and then in the default "~/.cdsapirc"
CDSAPI.retrieve(name, request, target)

juliohm

Thank you for the suggested improvement @ghyatzo , you can find review comments attached.

src/CDSAPI.jl

Project.toml

src/CDSAPI.jl

ghyatzo · 2025-01-15T12:53:13Z

I'll add some examples of this new usage in the readme later

src/CDSAPI.jl

juliohm · 2025-01-16T15:09:36Z

Yes. You can run these commands locally in the package folder. They will modify the Project.toml Em qui., 16 de jan. de 2025, 12:06, cschen ***@***.***> escreveu:

…

***@***.**** commented on this pull request. ------------------------------ In src/CDSAPI.jl <#58 (comment)> : > +if VERSION >= v"1.11" + using Base.ScopedValues +elseif VERSION >= v"1.8" + import Pkg + Pkg.activate(".") + Pkg.add("ScopedValues") + using ScopedValues +end Ok, so then just remove the Pkg.add("ScopedValues")? — Reply to this email directly, view it on GitHub <#58 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZQW3KKMMII4VVL7SALNBL2K7DGVAVCNFSM6AAAAABVG4LWPSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKNJWGQ2DCOBYGE> . You are receiving this because you commented.Message ID: ***@***.***>

Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>

and various renames

Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>

ghyatzo · 2025-01-16T15:22:43Z

OK, I've rebased onto master to have the other PRs.
I'll add some tests and examples in the README

ghyatzo · 2025-01-16T16:00:25Z

tests pass locally

juliohm · 2025-02-11T21:46:42Z

@ghyatzo the tests are failing. Should we continue the work on this PR?

ghyatzo · 2025-02-12T15:08:14Z

Hi, we encountered this issue in a previous PR #60 (comment)

Have you tried locally? apparently the PR branches do not have access to the secrets in the CI so the fail I saw were there, but locally with my credentials this branch pass all tests (at least, last time I checked, don't know if something else changed in the mean time)

juliohm · 2025-02-12T15:27:41Z

@ghyatzo there are pending review comments. I can try to run things locally again after they are considered.

renamed the scoped values to the uppercase variant

test/runtests.jl

src/CDSAPI.jl

README.md

Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>

ghyatzo · 2025-02-20T14:00:51Z

ok, sorry for the sloppy last few iterations, should have addressed the latest issues. I've merged master into the PR and tests pass locally.
Let me know if you need anything else

README.md

src/CDSAPI.jl

Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>

Enforce presence of key and url in file.

Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>

juliohm · 2025-02-24T11:30:33Z

Tests are passing locally for me, but failing on GitHub Actions. Some of the build failures point to missing credentials.

ghyatzo · 2025-02-24T12:01:28Z

see: #60 (comment)
and: https://stackoverflow.com/questions/73866908/github-actions-main-repository-secret-not-picked-up-from-pull-request-build

juliohm · 2025-02-24T12:07:09Z

see: #60 (comment) and: https://stackoverflow.com/questions/73866908/github-actions-main-repository-secret-not-picked-up-from-pull-request-build

I always forget. Sorry for the noise. Merging it.

juliohm requested changes Jan 15, 2025

View reviewed changes

juliohm reviewed Jan 15, 2025

View reviewed changes

src/CDSAPI.jl Outdated Show resolved Hide resolved

juliohm reviewed Jan 15, 2025

View reviewed changes

src/CDSAPI.jl Outdated Show resolved Hide resolved

juliohm reviewed Jan 15, 2025

View reviewed changes

Project.toml Outdated Show resolved Hide resolved

juliohm reviewed Jan 15, 2025

View reviewed changes

src/CDSAPI.jl Show resolved Hide resolved

juliohm mentioned this pull request Jan 16, 2025

allow for passing directly the python dict string as response body #59

Merged

juliohm reviewed Jan 16, 2025

View reviewed changes

src/CDSAPI.jl Outdated Show resolved Hide resolved

ghyatzo and others added 13 commits January 16, 2025 16:16

adds scoped values credential to support multiple endpoints.

14ef047

fix naming

bc31426

Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>

clearer check

3cfd2d5

Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>

rename CDScredentials

559d662

Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>

WIP: for now adding compatibility for 1.11 only.

8cc75b7

fix: better docs of credentials, moved the check inside the credentials.

41c6caf

and various renames

better docstring

dcb997e

Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>

add logic for ScopedValues compat

2c13a25

fix wrong string interpolation of $HOME

94bc65d

fix: logic error. auth[] is valid only if both entries are non empty.

70e812f

fix: even better logic. clearer.

3a52887

add ScopedValues as a direct Dependency

664ec81

fix: leftover

20358b0

ghyatzo added 3 commits January 16, 2025 16:57

added README entry about multiple token use with examples.

d4f10d7

export with from ScopedValues, so it is available when using CDSAPI

361b3d6

added tests

cc27e08

clarified the readme and removed references to credentialsfromfile

0ed846d

renamed the scoped values to the uppercase variant

juliohm reviewed Feb 20, 2025

View reviewed changes

test/runtests.jl Outdated Show resolved Hide resolved

juliohm reviewed Feb 20, 2025

View reviewed changes

src/CDSAPI.jl Outdated Show resolved Hide resolved

juliohm reviewed Feb 20, 2025

View reviewed changes

src/CDSAPI.jl Show resolved Hide resolved

juliohm reviewed Feb 20, 2025

View reviewed changes

README.md Show resolved Hide resolved

juliohm reviewed Feb 20, 2025

View reviewed changes

README.md Show resolved Hide resolved

ghyatzo and others added 4 commits February 20, 2025 14:44

Update README.md

0620f52

Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>

Update README.md

aca5093

Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>

default url as based, fix tests, fix leftover

359a846

Merge branch 'master' into scoped-auth

6dd7238

juliohm reviewed Feb 20, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

juliohm reviewed Feb 20, 2025

View reviewed changes

src/CDSAPI.jl Outdated Show resolved Hide resolved

juliohm reviewed Feb 20, 2025

View reviewed changes

src/CDSAPI.jl Outdated Show resolved Hide resolved

juliohm reviewed Feb 20, 2025

View reviewed changes

src/CDSAPI.jl Outdated Show resolved Hide resolved

juliohm reviewed Feb 20, 2025

View reviewed changes

src/CDSAPI.jl Outdated Show resolved Hide resolved

juliohm reviewed Feb 20, 2025

View reviewed changes

src/CDSAPI.jl Outdated Show resolved Hide resolved

juliohm reviewed Feb 20, 2025

View reviewed changes

src/CDSAPI.jl Outdated Show resolved Hide resolved

juliohm reviewed Feb 20, 2025

View reviewed changes

src/CDSAPI.jl Outdated Show resolved Hide resolved

ghyatzo and others added 4 commits February 21, 2025 17:53

typos and whitespace

f6f54f6

Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>

revert default url in scoped value.

a44d217

Enforce presence of key and url in file.

Apply suggestions from code review

e0baca5

Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>

Final adjustments

58bc951

juliohm approved these changes Feb 24, 2025

View reviewed changes

juliohm merged commit b48d3ed into JuliaClimate:master Feb 24, 2025
0 of 6 checks passed

ghyatzo deleted the scoped-auth branch February 24, 2025 12:12

Comments

Conversation

ghyatzo commented Jan 15, 2025

Uh oh!

juliohm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghyatzo commented Jan 15, 2025

Uh oh!

Uh oh!

juliohm commented Jan 16, 2025 via email

Uh oh!

ghyatzo commented Jan 16, 2025

Uh oh!

ghyatzo commented Jan 16, 2025

Uh oh!

juliohm commented Feb 11, 2025

Uh oh!

ghyatzo commented Feb 12, 2025

Uh oh!

juliohm commented Feb 12, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghyatzo commented Feb 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juliohm commented Feb 24, 2025

Uh oh!

ghyatzo commented Feb 24, 2025

Uh oh!

juliohm commented Feb 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants